Skip to content

Conversation

@teofr
Copy link
Contributor

@teofr teofr commented Jan 6, 2026

Originally I wanted to stop wrapping VersionSpecifier in an Option, and parse it as Always if it's not present, but that brings some issues with the span information, and we'd need to differentiate between an Always with span (explicit) and one without (implicit).

However, I think having the default value representable is good to simplify some algorithms, in particular, the information of what a missing field on the language definition means is now explicit.

I also added some documentation here and there.

@teofr teofr requested review from a team as code owners January 6, 2026 12:27
@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2026

⚠️ No Changeset found

Latest commit: abf58c0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@teofr teofr changed the title Added some docs and a default value for VersionSpecifier Added a default value for VersionSpecifier and some documentation Jan 7, 2026
@teofr teofr force-pushed the teofr/non-optional-version-spec branch from de31e98 to eff04aa Compare January 7, 2026 11:13
Copy link
Contributor

@ggiraldez ggiraldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions and suggestions.

@teofr teofr requested a review from ggiraldez January 19, 2026 10:14
@teofr teofr force-pushed the teofr/non-optional-version-spec branch from 268fd86 to 2745ec0 Compare January 19, 2026 10:15
Copy link
Contributor

@ggiraldez ggiraldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any more particular observations, but a more general question: do we need to differentiate between absent version specifier and Always? I don't see a reason to keep both possible states, and in that case we should change all other instances (eg. in StructItem, EnumItem, etc) of VersionSpecifier to remove the Option<>.

@teofr teofr force-pushed the teofr/non-optional-version-spec branch from 2745ec0 to 8961b0d Compare January 21, 2026 15:38
@teofr teofr requested a review from OmarTawfik January 21, 2026 15:50
@teofr
Copy link
Contributor Author

teofr commented Jan 21, 2026

(...) do we need to differentiate between absent version specifier and Always? (...)
@ggiraldez

The main reason is for the code generation engine, we expect most items of a parsed AST (of a language definition) to always have a span, most of the logic to derive spanned types works that way. So a SpannedVersionSpecifier has a Span, always. An Option<SpannedVersionSpecifier> may not have a span (if it's absent).

As far as I could see we treat Option<...> as special cases sometimes, I think is possible to have similar logic for Default structs (like the new VersionSpecifier) but you'd need to have some way of either differentiating between an explicit Always and an implicit one, or guarantees that no Always will need a span (no error messages).

What this change does allow is that further down the stream, for example when building a lexer, we can simply use .unwrap_or_default().

I agree it'd be a bigger win in ergonomics if we could get rid of the Option entirely, but it needs more work.

@ggiraldez
Copy link
Contributor

I agree it'd be a bigger win in ergonomics if we could get rid of the Option entirely, but it needs more work.

I see. I wasn't considering the model generation layer (partly because I haven't explored that part of the code sufficiently) and was thinking in terms of the model as a result, but they are tightly coupled. Thanks for the explanation!

language_v2_macros::compile!(Language(
name = Solidity,
binding_rules_file = "crates/solidity/inputs/language/bindings/rules.msgb",
root_item = SourceUnit,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing crates/solidity-v2/inputs/language/bindings/rules.msgb as well.

@teofr teofr force-pushed the teofr/non-optional-version-spec branch from bdb766c to abf58c0 Compare February 2, 2026 11:27
@teofr teofr enabled auto-merge February 2, 2026 11:36
@teofr teofr added this pull request to the merge queue Feb 2, 2026
Merged via the queue into main with commit 268c53d Feb 2, 2026
4 checks passed
@teofr teofr deleted the teofr/non-optional-version-spec branch February 2, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants